Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add an API for TLS 1.3 exporter #4230

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Add an API for TLS 1.3 exporter #4230

merged 1 commit into from
Oct 4, 2023

Conversation

Mark-Simulacrum
Copy link
Contributor

@Mark-Simulacrum Mark-Simulacrum commented Sep 29, 2023 •
edited
Loading

Description of changes:

This adds a new public API that exposes the TLS-Exporter function defined in RFC 8446. This API is used by consumers that wish to use the shared secret established from the TLS 1.3 handshake in order to perform out-of-band operations (e.g., custom protocol). The API does not expose the underlying master exporter secret to callers.

Call-outs:

  • Early exporter functionality is not added. The RFC recommends that this is exposed via a separate interface, so this doesn't seem like a problem.
  • TLS 1.2 support not implemented -- this might be best in a separate API, or I think it should be possible to add to the current API if desired. This is enforced at runtime.
  • This increases the maximum allowed label length in s2n_hkdf_expand_label from 12 to 249. This matches OpenSSL behavior for its similar internal API, and is required for exporter functionality which frequently contains longer labels due to the nearly-required "EXPERIMENTAL EXPORTER " prefix which is already longer.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)?

  • New tests are added ensuring that the exporter master secret matches the values in the RFC 8448 (for the simple 1-RTT handshake).
  • Additionally a test is added ensuring the new public API continues to produce the same value going forward. This value is validated against OpenSSL by running an OpenSSL client against an s2n server and making sure that exporting produces the same full output in both after a handshake.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

camshaft
camshaft reviewed Sep 29, 2023
View reviewed changes
api/s2n.h Outdated Show resolved Hide resolved
tls/s2n_tls13_secrets.c Outdated Show resolved Hide resolved
@Mark-Simulacrum Mark-Simulacrum force-pushed the support-exporter branch 3 times, most recently from a7eec2a to 7d98afd Compare September 29, 2023 21:04
lrstewart
lrstewart reviewed Sep 29, 2023
View reviewed changes
api/s2n.h Outdated Show resolved Hide resolved
tls/s2n_tls13_secrets.c Outdated Show resolved Hide resolved
tls/s2n_tls13_secrets.c Outdated Show resolved Hide resolved
tls/s2n_tls13_secrets.c Outdated Show resolved Hide resolved
tls/s2n_tls13_secrets.c Outdated Show resolved Hide resolved
tls/s2n_tls13_secrets.c Outdated Show resolved Hide resolved
lrstewart
lrstewart reviewed Sep 29, 2023
View reviewed changes
tests/unit/s2n_tls13_secrets_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_tls13_secrets_test.c Outdated Show resolved Hide resolved
camshaft
camshaft reviewed Oct 2, 2023
View reviewed changes
api/s2n.h Outdated Show resolved Hide resolved
lrstewart
lrstewart reviewed Oct 2, 2023
View reviewed changes
tls/s2n_tls13_secrets.c Outdated Show resolved Hide resolved
tls/s2n_tls13_secrets.c Outdated
Comment on lines 740 to 765
struct s2n_blob digest = EMPTY_CONTEXT(hmac_alg);

POSIX_GUARD(s2n_hash_init(&hash, hash_alg));
POSIX_GUARD(s2n_hash_update(&hash, context, context_length));
POSIX_GUARD(s2n_hash_digest(&hash, digest.data, digest.size));
Copy link
Contributor

@lrstewart lrstewart Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, this doesn't look right. EMPTY_CONTEXT isn't equivalent to an zero-inited blob of the right size. It actually has a very specific value that we calculate when s2n is initialized:

s2n-tls/tls/s2n_tls13_secrets.c

Lines 85 to 101 in ab7bfce

S2N_RESULT s2n_tls13_empty_transcripts_init()
{
DEFER_CLEANUP(struct s2n_hash_state hash = { 0 }, s2n_hash_free);
RESULT_GUARD_POSIX(s2n_hash_new(&hash));
s2n_hash_algorithm hash_alg = S2N_HASH_NONE;
for (size_t i = 0; i < s2n_array_len(supported_hmacs); i++) {
s2n_hmac_algorithm hmac_alg = supported_hmacs[i];
struct s2n_blob digest = EMPTY_CONTEXT(hmac_alg);
RESULT_GUARD_POSIX(s2n_hmac_hash_alg(hmac_alg, &hash_alg));
RESULT_GUARD_POSIX(s2n_hash_init(&hash, hash_alg));
RESULT_GUARD_POSIX(s2n_hash_digest(&hash, digest.data, digest.size));
}
return S2N_RESULT_OK;
}
Aren't you overwriting that value here? That would break any future method that used EMPTY_CONTEXT.

This is another fun consequence of the blob limitations from https://github.com/aws/s2n-tls/pull/4230/files#r1343030637 And apparently EMPTY_CONTEXT needs a scary warning, or we might just want to calculate it repeatedly at runtime to avoid this problem :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the immediate bug, also filed #4233 for follow-up.

lrstewart
lrstewart reviewed Oct 2, 2023
View reviewed changes
tests/unit/s2n_tls13_secrets_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_tls13_secrets_test.c Show resolved Hide resolved
crypto/s2n_hkdf.c Outdated Show resolved Hide resolved
tls/s2n_tls13_secrets.c Outdated Show resolved Hide resolved
tls/s2n_tls13_secrets.c Outdated Show resolved Hide resolved
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Oct 2, 2023
Open
lrstewart
lrstewart reviewed Oct 3, 2023
View reviewed changes
tests/unit/s2n_tls13_secrets_test.c Show resolved Hide resolved
tests/unit/s2n_tls13_secrets_test.c Outdated Show resolved Hide resolved
tls/s2n_tls13_secrets.c Outdated Show resolved Hide resolved
tls/s2n_tls13_secrets.c Outdated Show resolved Hide resolved
@Mark-Simulacrum Mark-Simulacrum force-pushed the support-exporter branch 2 times, most recently from e169f5a to 5052273 Compare October 4, 2023 00:07
@camshaft camshaft enabled auto-merge (squash) October 4, 2023 01:29
@Mark-Simulacrum
Add support for exporting symmetric keys from connections
e6c57d8 
This is currently only supported for TLS 1.3 and is standard compliant,
using the TLS-Exporter function defined by RFC 8446:
https://www.rfc-editor.org/rfc/rfc8446#section-7.5
auto-merge was automatically disabled October 4, 2023 02:10

Head branch was pushed to by a user without write access

@camshaft camshaft merged commit b15a1ba into aws:main Oct 4, 2023
23 checks passed
@Mark-Simulacrum Mark-Simulacrum deleted the support-exporter branch October 4, 2023 15:56
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Oct 4, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lrstewart lrstewart lrstewart approved these changes

@camshaft camshaft camshaft approved these changes

@goatgoose goatgoose Awaiting requested review from goatgoose

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Mark-Simulacrum @camshaft @lrstewart